perf(wacore): drop the proto PartialEq anchor from the skdm-only check - #846
Merged
Merged
Conversation
The slow path compared the stripped message against Message::default(), which was the only production caller of prost's derived PartialEq and kept the eq impls for the entire message tree alive in every binary (94 KiB across 148 functions). Under proto2 presence rules a field only contributes encoded bytes when set, so encoded_len == 0 is the same predicate; it reuses the codec tree already pinned by waproto::codec. Measured on the release bin: .text 11.65 MiB to 11.55 MiB (-102 KiB), proto eq impls 148 to 0. Applies to stable and nightly builds alike.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR optimizes message validation in ChangesSender-key distribution detection optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cargo llvm-linesflagged prost's derivedPartialEqas the largest codegen item in wacore (Message::eqalone is 4.2k IR lines, with the whole nested tree behind it). In the release binary that materializes as 94 KiB of.textacross 148eqfunctions, and symbol tracing showed exactly one production caller keeping all of it alive: the skdm-only check inwacore::messages, which compares the carrier-stripped message againstMessage::default().Change
encoded_len() == 0is the same predicate: under proto2 presence rules a field only contributes encoded bytes when it is set, so zero encoded length means every remaining field is unset, which is exactly equality withMessage::default(). It routes throughwaproto::codec::message_encoded_len, the tree already pinned by #842, so no new codegen appears anywhere.Cost-wise the paths are equivalent where it matters: for a true skdm-only message both walks visit every (unset) field once; the fast path above the slow path still short-circuits typical content messages before either runs.
Measured
Release bin:
.text11.65 MiB -> 11.55 MiB (-102 KiB); protoeqimpls 148 -> 0. Unlike the Dockerfile flags, this one applies to stable and nightly consumers alike.Tests
wacore (1008) and lib (830) suites pass; the skdm-only behavior is locked by the existing dedicated tests.